-
-
Notifications
You must be signed in to change notification settings - Fork 628
refactor: less use of PartialBlocks and type cleanup #2111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
packages/core/src/api/blockManipulation/commands/insertBlocks/insertBlocks.ts
Outdated
Show resolved
Hide resolved
| * @returns an {@link OccupancyGrid} | ||
| */ | ||
| export function getTableCellOccupancyGrid( | ||
| block: BlockFromConfigNoChildren<DefaultBlockSchema["table"], any, any>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopped exporting BlockFromConfigNoChildren so we have more consistency in the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are cleaning up types, I believe that:
BlockFromConfig<DefaultBlockSchema["table"], any, any>Should only be:
BlockFromConfig<DefaultBlockSchema["table"]>Most of the time we don't care about the other parameters, and have to remove their defaults with any. This goes for most instances of B, I, S
packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| public blocksToHTMLLossy( | ||
| blocks: PartialBlock<BSchema, ISchema, SSchema>[] = this.document, | ||
| blocks: Block<BSchema, ISchema, SSchema>[] = this.document, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breaking API change, should be documented in PR / release notes
| } | ||
|
|
||
| // Hide handles if the table block has been removed. | ||
| this.state.block = this.editor.getBlock(this.state.block.id)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: In the old version we were potentially setting this.state.block to undefined. Now, it will keep the old (unavailable block). can this break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't clear to me why this was modified, but it certainly is a change in behavior that would need to be validated
packages/core/src/api/blockManipulation/commands/insertBlocks/insertBlocks.ts
Outdated
Show resolved
Hide resolved
packages/core/src/api/blockManipulation/commands/updateBlock/updateBlock.ts
Outdated
Show resolved
Hide resolved
| * @returns an {@link OccupancyGrid} | ||
| */ | ||
| export function getTableCellOccupancyGrid( | ||
| block: BlockFromConfigNoChildren<DefaultBlockSchema["table"], any, any>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are cleaning up types, I believe that:
BlockFromConfig<DefaultBlockSchema["table"], any, any>Should only be:
BlockFromConfig<DefaultBlockSchema["table"]>Most of the time we don't care about the other parameters, and have to remove their defaults with any. This goes for most instances of B, I, S
packages/core/src/extensions/TableHandles/TableHandlesPlugin.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Hide handles if the table block has been removed. | ||
| this.state.block = this.editor.getBlock(this.state.block.id)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't clear to me why this was modified, but it certainly is a change in behavior that would need to be validated
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
Summary
Clean types and Partial / full block concepts
Rationale
#2089 (review)
Changes
PartialBlockinternally, where it's not necessaryImpact
Testing
Should mostly be covered by existing unit tests
Screenshots/Video
Checklist
Additional Notes
TODO:
Follow up: